-
Notifications
You must be signed in to change notification settings - Fork 13.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MINOR: fixing JavaDocs and other cleanup #17207
base: trunk
Are you sure you want to change the base?
Conversation
@@ -1452,7 +1456,7 @@ public CloseOptions leaveGroup(final boolean leaveGroup) { | |||
* This will block until all threads have stopped. | |||
*/ | |||
public void close() { | |||
close(Long.MAX_VALUE, false); | |||
close(Optional.empty(), false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing this to Optional
to unify the log message (cf other changes).
"and the registered exception handler opted to " + action + "." + | ||
" The streams client is going to shut down now. ", throwable); | ||
log.error(String.format( | ||
"Encountered the following exception during processing and the registered exception handler" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pardon me, why not using {}
placeholder directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjsax thanks for this patch. overall LGTM
It seems StreamConfig
prefers to use String.format
rather than slf4j "parameterized messages". It is cool to have consistent code style, but using slf4j format is more simple and fast (https://www.slf4j.org/faq.html#logging_performance).
final long timeoutMs; | ||
if (timeout.isPresent()) { | ||
timeoutMs = timeout.get(); | ||
log.debug("Stopping Streams client with timeoutMillis = {} ms.", timeoutMs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: there is "ms" already, so timeoutMillis
could be simplified to timeout
@@ -1309,7 +1309,11 @@ public static boolean getBoolean(final Map<String, Object> configs, final String | |||
} else if (value instanceof String) { | |||
return Boolean.parseBoolean((String) value); | |||
} else { | |||
log.warn("Invalid value (" + value + ") on internal configuration '" + key + "'. Please specify a true/false value."); | |||
log.warn(String.format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we use placeholder?
log.warn(
"Invalid value ({}) on internal configuration '{}}'. Please specify a true/false value.",
value,
key
);
@@ -1321,7 +1325,11 @@ public static long getLong(final Map<String, Object> configs, final String key, | |||
} else if (value instanceof String) { | |||
return Long.parseLong((String) value); | |||
} else { | |||
log.warn("Invalid value (" + value + ") on internal configuration '" + key + "'. Please specify a numeric value."); | |||
log.warn(String.format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -1331,7 +1339,11 @@ public static String getString(final Map<String, Object> configs, final String k | |||
if (value instanceof String) { | |||
return (String) value; | |||
} else { | |||
log.warn("Invalid value (" + value + ") on internal configuration '" + key + "'. Please specify a String value."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
Thing IntelliJ highlighted...